src/instantsend/instantsend.h (1)

30-30: Consider including primitives/transaction.h instead of re-declaring CTransactionRef.

This file-level typedef duplicates the canonical definition from primitives/transaction.h. If the canonical typedef ever changes (e.g., to use a different smart pointer type), this local copy would silently diverge.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/instantsend.h` at line 30, The file-level typedef duplicates
the canonical CTransactionRef; remove the local typedef in instantsend.h and
include the canonical header instead: add an `#include` of
primitives/transaction.h and delete the line "typedef std::shared_ptr<const
CTransaction> CTransactionRef;" so the file uses the single source-of-truth
CTransactionRef definition (referenced here as CTransactionRef in
instantsend.h).

src/instantsend/net_instantsend.cpp (5)

74-88: ResolveCycleHeight duplicates the fallback already performed inside GetBlockHeight.

GetBlockHeight (lines 40–55) already checks the cache, falls back to LookupBlockIndex, caches the result, and returns the height. If it returns std::nullopt, the block simply isn't known. The second LookupBlockIndex call on line 81 will hit the same state under the same cs_main lock and produce the same nullptr.

Suggested simplification

 std::optional<int> NetInstantSend::ResolveCycleHeight(const uint256& cycle_hash)
 {
-    auto cycle_height = GetBlockHeight(m_is_manager, m_chainstate, cycle_hash);
-    if (cycle_height) {
-        return cycle_height;
-    }
-
-    const auto block_index = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(cycle_hash));
-    if (block_index == nullptr) {
-        return std::nullopt;
-    }
-
-    m_is_manager.CacheBlockHeight(block_index);
-    return block_index->nHeight;
+    return GetBlockHeight(m_is_manager, m_chainstate, cycle_hash);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 74 - 88, ResolveCycleHeight
duplicates the fallback logic already implemented in GetBlockHeight: call
GetBlockHeight(m_is_manager, m_chainstate, cycle_hash) and return its result
directly instead of performing a second WITH_LOCK +
m_chainstate.m_blockman.LookupBlockIndex and manual caching; remove the extra
LookupBlockIndex and m_is_manager.CacheBlockHeight usage from ResolveCycleHeight
so it simply returns the std::optional<int> produced by GetBlockHeight.

---

25-31: Forward-declaring node::GetTransaction to break a header dependency is fragile.

If the signature in node/transaction.h ever changes, this forward declaration will silently diverge, leading to linker errors or undefined behavior. Consider adding a static_assert or a comment pointing to the canonical declaration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 25 - 31, The forward
declaration of node::GetTransaction (CTransactionRef GetTransaction(const
CBlockIndex* const, const CTxMemPool*, const uint256&, const Consensus::Params&,
uint256&)) is fragile; update the file to either include the canonical
declaration from node/transaction.h or add a compile-time check to ensure the
signature matches: add a static_assert that compares
decltype(&node::GetTransaction) to the expected function pointer type (or
otherwise references the exact signature), and/or add a clear comment pointing
to node/transaction.h as the authoritative declaration to prevent silent
divergence.

---

353-415: ProcessInstantSendLock looks correct; one observation on the dual-lookup pattern.

The method first calls GetTransaction(nullptr, &m_mempool, ...) which may return a mined tx with hashBlock set, then calls GetBlockHeight which may miss the cache and do a LookupBlockIndex. If the block is known, lines 370–375 do a third lookup via LookupBlockIndex. Consider consolidating: if GetTransaction returns a non-null hashBlock, you could do the LookupBlockIndex once and pass the result to both the height-caching path and the ChainLock check.

This is a minor efficiency nit and not a correctness issue.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 353 - 415,
ProcessInstantSendLock performs multiple block-index lookups: after
GetTransaction and GetBlockHeight, code calls LookupBlockIndex again (in the
block handling found_transaction) causing a redundant third lookup; consolidate
by performing a single LookupBlockIndex once when the transaction is found and
minedHeight is empty (capture result in a local pindexMined), call
m_is_manager.CacheBlockHeight(pindexMined) and set minedHeight from
pindexMined->nHeight, then use that minedHeight for the subsequent
m_chainlocks.HasChainLock(hash, hashBlock) check and for WriteNewISLock; update
the block that currently does LookupBlockIndex (the code around lines using
WITH_LOCK(::cs_main) and pindexMined) to reuse this single lookup and remove the
duplicate lookup path so LookupBlockIndex is only called once per code path.

---

591-591: Note: IsKnownTx semantics are inverted from what the name suggests (see instantsend.cpp comment).

At line 591, isLockedTxKnown will be true when the islock is not in pendingNoTxInstantSendLocks (meaning we already have the transaction). This works correctly in context but the naming is confusing. See related comment on instantsend.cpp:273-277.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` at line 591, The variable
isLockedTxKnown is misleading because m_is_manager.IsKnownTx(islockHash) returns
true when the IS lock is NOT in pendingNoTxInstantSendLocks (i.e., we already
have the transaction); rename or invert the boolean to match semantics: either
rename isLockedTxKnown to isLockedTxMissing or isLockedTxPresent (choose a name
that matches the actual meaning), or assign with a negation (e.g., bool
isLockedTxMissing = !m_is_manager.IsKnownTx(islockHash)) and update all
subsequent uses accordingly (references around net_instantsend.cpp where
isLockedTxKnown is checked). Ensure consistency with the comment in
instantsend.cpp about IsKnownTx semantics.

---

222-241: The else branch with assert(false) is unreachable thanks to the requires clause.

The C++20 requires constraint on line 224 already limits T to CTxIn or COutPoint, so the else branch on line 235 can never be reached. This is harmless but adds dead code.

Simplified version

 template <typename T>
     requires std::same_as<T, CTxIn> || std::same_as<T, COutPoint>
 Uint256HashSet GetIdsFromLockable(const std::vector<T>& vec)
 {
     Uint256HashSet ret{};
     if (vec.empty()) return ret;
     ret.reserve(vec.size());
     for (const auto& in : vec) {
         if constexpr (std::is_same_v<T, COutPoint>) {
             ret.emplace(instantsend::GenInputLockRequestId(in));
-        } else if constexpr (std::is_same_v<T, CTxIn>) {
+        } else {
+            static_assert(std::is_same_v<T, CTxIn>);
             ret.emplace(instantsend::GenInputLockRequestId(in.prevout));
-        } else {
-            assert(false);
         }
     }
     return ret;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 222 - 241, The unreachable
else branch in the template function GetIdsFromLockable (anonymous namespace)
should be removed: since the requires clause already constrains T to CTxIn or
COutPoint, delete the final "else { assert(false); }" block and keep only the
two constexpr branches that call instantsend::GenInputLockRequestId (one for
COutPoint and one for CTxIn.prevout); this removes dead code without changing
behavior.

src/instantsend/instantsend.cpp (2)

273-277: IsKnownTx name is misleading — it returns true when the islock is absent from the no-tx pending map.

The method returns true when the hash is not found in pendingNoTxInstantSendLocks, meaning "we already have the transaction for this islock." While the logic is correct in context (used in ResolveBlockConflicts to decide whether to activate best chain), the name reads as the opposite of what the lookup does. A name like HasTransactionForLock or IsNotAwaitingTx would be clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/instantsend.cpp` around lines 273 - 277, The method
CInstantSendManager::IsKnownTx is misleading because it returns true when
islockHash is NOT present in pendingNoTxInstantSendLocks; rename the method to
something like HasTransactionForLock or IsNotAwaitingTx and update all call
sites (e.g., usages in ResolveBlockConflicts) to the new name, preserving the
current boolean logic (return pendingNoTxInstantSendLocks.find(islockHash) ==
pendingNoTxInstantSendLocks.end()); alternatively, if you prefer to keep the
name, invert the return and adjust callers to match the new semantics—ensure
consistency between the method name and its boolean meaning across the codebase.

---

126-144: Linear scan in AttachISLockToTx could be replaced with a reverse index.

AttachISLockToTx iterates the entire pendingNoTxInstantSendLocks map to find a match by txid. If this map grows large, this becomes O(n). Consider maintaining a secondary txid → islockHash index for O(1) lookups. Not urgent for this PR, but worth noting for future optimization.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/instantsend.cpp` around lines 126 - 144, AttachISLockToTx
currently does an O(n) linear scan over pendingNoTxInstantSendLocks to find an
islock by tx->GetHash(); add a secondary unordered_map/index (e.g.,
pendingNoTxIndex mapping txid -> islockHash/key) and maintain it wherever
entries are inserted or erased so AttachISLockToTx can do an O(1) lookup: find
the key via pendingNoTxIndex[txid], move the entry from
pendingNoTxInstantSendLocks to pendingInstantSendLocks (as currently done),
erase from pendingNoTxInstantSendLocks and pendingNoTxIndex, and return the
islock; ensure consistency on all code paths that add/remove from
pendingNoTxInstantSendLocks so the new index stays in sync.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

src/instantsend/signing.cpp (1)

207-237: ⚠️ Potential issue | 🟠 Major

Guard null hashBlock before computing parent age and use cache result correctly.

At Line 234, nTxAge is computed even if hashBlock is null (with blockHeight left at 0), which can incorrectly allow locking. Also, Line 209-Line 222 still does LookupBlockIndex even after a cache hit.

💡 Suggested fix

-    int blockHeight{0};
-    if (!hashBlock.IsNull()) {
-        auto ret = m_isman.GetCachedHeight(hashBlock);
-        if (ret) blockHeight = *ret;
-
-        const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
-        if (pindex == nullptr) {
-            if (printDebug) {
-                LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
-                         __func__, txHash.ToString(), outpoint.hash.ToString());
-            }
-            return false;
-        }
-        m_isman.CacheBlockHeight(pindex);
-        blockHeight = pindex->nHeight;
-    }
+    if (hashBlock.IsNull()) {
+        if (printDebug) {
+            LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: parent TX %s is not mined yet\n",
+                     __func__, txHash.ToString(), outpoint.hash.ToString());
+        }
+        return false;
+    }
+
+    int blockHeight{0};
+    if (const auto cached_height = m_isman.GetCachedHeight(hashBlock)) {
+        blockHeight = *cached_height;
+    } else {
+        const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
+        if (pindex == nullptr) {
+            if (printDebug) {
+                LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
+                         __func__, txHash.ToString(), outpoint.hash.ToString());
+            }
+            return false;
+        }
+        m_isman.CacheBlockHeight(pindex);
+        blockHeight = pindex->nHeight;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/signing.cpp` around lines 207 - 237, Guard against hashBlock
being null and honor cached height to avoid unnecessary LookupBlockIndex: call
auto ret = m_isman.GetCachedHeight(hashBlock) only if !hashBlock.IsNull(), and
if ret has a value set blockHeight = *ret and skip the
LookupBlockIndex/CacheBlockHeight work; if hashBlock.IsNull() return false (or
skip age checks) before computing nTxAge so blockHeight isn't treated as 0, and
keep the subsequent check using m_chainlocks.HasChainLock(blockHeight,
hashBlock) unchanged.

src/instantsend/net_instantsend.h (1)

110-110: Consider in-class initialization for the nullable pointer.

Per project conventions for header declarations, POD types should use in-class initialization. Since m_signer is documented as nullable (non-null only for masternodes), consider initializing it explicitly.

Proposed change

-    instantsend::InstantSendSigner* m_signer; // non-null only for masternode
+    instantsend::InstantSendSigner* m_signer{nullptr}; // non-null only for masternode

Based on learnings: "In header files, prefer in-class initialization for POD types (e.g., bool m_flag = false; int m_value = 0)."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.h` at line 110, The member pointer m_signer
is nullable but currently uninitialized in the header; initialize it in-class to
a clear null value (e.g., = nullptr) to follow POD in-class initialization
conventions and avoid indeterminate pointer state—update the declaration of
instantsend::InstantSendSigner* m_signer to include the in-class initializer
(nullptr) so code that checks m_signer can rely on a defined initial value.

src/instantsend/instantsend.cpp (2)

293-297: Method name could be clearer given its semantics.

IsKnownTx takes an islockHash parameter but returns whether the corresponding transaction is known. Consider renaming to IsTransactionKnownForLock or adding a brief doc comment to clarify that it returns true when the ISLOCK's transaction has been seen (i.e., the ISLOCK is not in the "pending without transaction" map).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/instantsend.cpp` around lines 293 - 297, Rename or clarify
CInstantSendManager::IsKnownTx to make its semantics explicit: either rename the
method to IsTransactionKnownForLock (or IsTxKnownForIslock) and update all
callers, or add a one-line doc comment above IsKnownTx stating that given an
islockHash it returns true when the ISLOCK's transaction has been seen (i.e.,
the islockHash is NOT present in pendingNoTxInstantSendLocks). Ensure the
implementation still locks cs_pendingLocks and uses
pendingNoTxInstantSendLocks.find(islockHash) ==
pendingNoTxInstantSendLocks.end(), and update any unit tests or usages to call
the new name if you rename the method.

---

146-164: Loop iteration can be simplified.

The manual iterator loop can be replaced with std::find_if or a range-based approach for clarity. Also, once a match is found and erased, the method returns immediately, so the ++it is unreachable after the erase.

Proposed simplification

 instantsend::InstantSendLockPtr CInstantSendManager::AttachISLockToTx(const CTransactionRef& tx)
 {
-    instantsend::InstantSendLockPtr ret_islock{nullptr};
     LOCK(cs_pendingLocks);
-    auto it = pendingNoTxInstantSendLocks.begin();
-    while (it != pendingNoTxInstantSendLocks.end()) {
+    for (auto it = pendingNoTxInstantSendLocks.begin(); it != pendingNoTxInstantSendLocks.end(); ++it) {
         if (it->second.islock->txid == tx->GetHash()) {
-            // we received an islock earlier, let's put it back into pending and verify/lock
             LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__,
                      tx->GetHash().ToString(), it->first.ToString());
-            ret_islock = it->second.islock;
+            auto ret_islock = it->second.islock;
             pendingInstantSendLocks.try_emplace(it->first, it->second);
             pendingNoTxInstantSendLocks.erase(it);
             return ret_islock;
         }
-        ++it;
     }
-    return ret_islock; // not found, nullptr
+    return nullptr;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/instantsend.cpp` around lines 146 - 164, The manual iterator
loop in CInstantSendManager::AttachISLockToTx over pendingNoTxInstantSendLocks
can be simplified and clarified by using std::find_if (or a range-based for to
locate the matching entry) instead of manually advancing the iterator and
performing an erase-then-return (which makes the ++it after erase unreachable);
find the element where it->second.islock->txid == tx->GetHash(), then inside the
cs_pendingLocks lock assign ret_islock = it->second.islock, move the entry into
pendingInstantSendLocks (pendingInstantSendLocks.try_emplace(it->first,
it->second)), erase the element from pendingNoTxInstantSendLocks, and return
ret_islock.

src/instantsend/net_instantsend.cpp (2)

600-624: Fatal assertions on block invalidation/activation failures are intentional but aggressive.

The assert(false) calls after InvalidateBlock and ActivateBestChain failures will terminate the node. While the comments explain this is intentional for safety, consider whether logging a critical error and returning might be more appropriate for production resilience. However, given this is consensus-critical code handling ISLOCK conflicts, the fail-fast approach may be justified.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 600 - 624, The code
currently uses assert(false) after failures from
m_chainstate.InvalidateBlock(...) and m_chainstate.ActivateBestChain(...), which
will hard-crash the node; replace each assert(false) with a non-fatal error
path: LogPrintf a critical/error-level message including state.ToString()
(preserve the existing message), then cleanly abort processing by returning from
the enclosing function (or otherwise propagate the failure to the caller) so you
do not continue in an unsafe state; ensure any locks (e.g., ::cs_main) are
properly released and that activateBestChain/isLockedTxKnown logic is not
executed after the failure. Include references to BlockValidationState state,
pindex2, m_chainstate.InvalidateBlock, m_chainstate.ActivateBestChain, and the
existing LogPrintf calls when making the change.

---

222-241: Unreachable branch in constexpr if.

The else branch with assert(false) at line 236 is unreachable because the requires clause already constrains T to be either CTxIn or COutPoint. The compiler will reject any other type at compile time.

Proposed simplification

 template <typename T>
     requires std::same_as<T, CTxIn> || std::same_as<T, COutPoint>
 Uint256HashSet GetIdsFromLockable(const std::vector<T>& vec)
 {
     Uint256HashSet ret{};
     if (vec.empty()) return ret;
     ret.reserve(vec.size());
     for (const auto& in : vec) {
         if constexpr (std::is_same_v<T, COutPoint>) {
             ret.emplace(instantsend::GenInputLockRequestId(in));
-        } else if constexpr (std::is_same_v<T, CTxIn>) {
-            ret.emplace(instantsend::GenInputLockRequestId(in.prevout));
         } else {
-            assert(false);
+            ret.emplace(instantsend::GenInputLockRequestId(in.prevout));
         }
     }
     return ret;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` around lines 222 - 241, The function
template GetIdsFromLockable has an unreachable else branch (assert(false))
because the requires clause limits T to CTxIn or COutPoint; remove that
unreachable branch and simplify the constexpr branching: keep the if constexpr
(std::is_same_v<T, COutPoint>) {
ret.emplace(instantsend::GenInputLockRequestId(in)); } and make the other branch
a plain else that handles CTxIn
(ret.emplace(instantsend::GenInputLockRequestId(in.prevout));), deleting the
final assert; this keeps behavior the same while removing dead code in
GetIdsFromLockable.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro